Print output in json format when using global --json flag - #274
Print output in json format when using global --json flag#274vbmade2000 wants to merge 13 commits into
Conversation
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
|
@atimin The PR is still in WIP. Let me know your thoughts on approach taken in this PR. |
| // Do not output json if the command is "cp" | ||
| if matches.get_flag("json") && command != "cp" { | ||
| let json_error = json!({ | ||
| "status": "error", |
There was a problem hiding this comment.
reduct-rs sdk returns the ReductError with a status and an error message. I think we can attach it to the anyhow error and downcast here to get the status. See https://docs.rs/anyhow/latest/anyhow/trait.Context.html.
There was a problem hiding this comment.
Ah I didn't know that ReductError error has status code too. I'll update the code.
|
Hi @vbmade2000 , I think it's a good approach but we could try to improve the error handling without breaking the interfaces. |
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
|
@atimin I have updated code to return status code. Let me know if it looks good to you. I'll write tests for bucket once you approve the approach. |
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
| (reduct_err.status() as i32, reduct_err.message().to_string()) | ||
| } else { | ||
| // If not a ReductError, use 1 as unknown status | ||
| (1, err.to_string()) |
There was a problem hiding this comment.
Hi @vbmade2000 , sorry looks like I missed your request. Yes, we can continue with the following approach but it should be -1 to be compatible with ReductStore error codes: https://github.com/reductstore/reductstore/blob/661918503a8d762f599ad1a7589c23330fb6c464/reduct_base/src/error.rs#L23
There was a problem hiding this comment.
No probs. I'll keep that in mind.
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
317ad1a to
6d38d33
Compare
|
@atimin This one is ready for review. I don't know why the test is failing. It passes on my system. |
|
Closed by mistake. |
atimin
left a comment
There was a problem hiding this comment.
Hi @vbmade2000 ,
Let's add the flag into all the bucket commands not only "create". Please update the changelog file and the PR description.
Thank you!
The "cargo fmt" command is failing. You can use pre-commit to run the formatting automatically. |
Indeed. This is just for your approval. I don't want to do rework so I move forward after I get approval from you. |
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
4a6caa1 to
bef8867
Compare
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
bef8867 to
c23d785
Compare
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
a885ff2 to
b21cfa4
Compare
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
b21cfa4 to
c59c3c2
Compare
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
Signed-off-by: Malhar Vora <mlvora.2010@gmail.com>
|
@atimin I think this one is ready for review. The current failure of test is I think random because I haven't touched that part. I guess it should pass once you restart the flow. |
atimin
left a comment
There was a problem hiding this comment.
Hi @vbmade2000 , thank you for the PR.
Unfortunately, we should work on it a bit more. Please, see my comments.
The --json flag is supposed to be used in scripts and automations so must be muchine friendly first. Instead of:
{
"bucket": {
"name": "src",
"quota_type": "NONE",
"entry_count": 0,
"quota_size": "0 B",
"size": "0 B",
"max_block_size": "64.0 MB",
"blocks": 0,
"max_block_records": 1024,
"status": "✅ Ready",
"provisioned": "✓"
},
"entries": []
}
We should send:
{
"bucket": {
"name": "src",
"quota_type": "NONE",
"entry_count": 0,
"quota_size": 0,
"size": 0,
"max_block_size": 640000000,
"blocks": 0,
"max_block_records": 1024,
"status": "READY",
"provisioned": true,
},
"entries": []
}
Actually , just the JSON payload we receive from the backend.
Thank you!
| self.ca_cert.as_ref() | ||
| } | ||
|
|
||
| pub(crate) fn json(&self) -> Option<bool> { |
There was a problem hiding this comment.
I don't think it makes sense to use option for a boolean flag here. We print the json output or not -- no the 3rd state.
| } | ||
|
|
||
| fn print_list(ctx: &CliContext, bucket_list: BucketInfoList) { | ||
| fn print_list(ctx: &CliContext, bucket_list: BucketInfoList, is_json: bool) { |
There was a problem hiding this comment.
We have the JSON flag in the context, so there is no need to add an additional parameter.
| } | ||
|
|
||
| #[derive(Tabled)] | ||
| #[derive(Deserialize, Serialize, Tabled)] |
There was a problem hiding this comment.
Ideally, I would like to serialize the JSON data we receive from the backend in order to decouple the API and its table representation.
| } | ||
|
|
||
| fn print_full_list(ctx: &CliContext, bucket_list: BucketInfoList) { | ||
| fn print_full_list(ctx: &CliContext, bucket_list: BucketInfoList, is_json: bool) { |
There was a problem hiding this comment.
We have the flag in the context already.
| .await?; | ||
| } else { | ||
| remove_entire_bucket(ctx, args, &alias_or_url, &bucket_name).await?; | ||
| remove_entire_bucket(ctx, args, &alias_or_url, &bucket_name, is_json).await?; |
Closes #254
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
(Bug fix, feature, docs update, ...)
What was changed?
(Describe the changes)
Related issues
(Add links to related issues)
Does this PR introduce a breaking change?
(What changes might users need to make in their application due to this PR?)
Other information: